Review: Design concern — global OAUTH_ALLOWED_ROLES vs per-provider JSONUserRolesPath

OAUTH_ALLOWED_ROLES is a single global env var, but JSONUserRolesPath is configured per OAuth provider. This creates a problem in multi-provider setups:

• If you have Google OAuth (no roles path configured) + custom OAuth (with roles), Google users will always be denied when OAUTH_ALLOWED_ROLES is set, because hasAllowedRole() receives an empty roles slice and returns false.
• Different providers may use different role names, but there's no way to configure allowed roles per provider.

Consider either:

1. Making OAUTH_ALLOWED_ROLES a per-provider setting (stored alongside JSONUserRolesPath in the DB), or
2. Skipping the role check when the provider has no JSONUserRolesPath configured (i.e., only enforce when the provider actually returns roles)

Option 2 is simpler and would only require checking whether the provider's JSONUserRolesPath is non-empty before calling hasAllowedRole().

Additional notes

• Missing test coverage for hasAllowedRole() — roles_test.go tests extractRolesFromJSON but not the actual access-control gate function. This should have tests covering: empty config (allow all), matching role, no matching role, empty user roles with config set.

• Separator convention — OAUTH_ALLOWED_ROLES uses ; as separator, which is unusual for env vars. Consider using , for consistency. The role string splitting in extractRolesFromJSON tries ; then , which adds ambiguity.

• navigateJSONPath duplicates app/pkg/jsonq — The dot-notation path traversal in navigateJSONPath() reimplements what jsonq.Query.get() already does (app/pkg/jsonq/jsonq.go:89). The array extraction logic (roles[].id) is genuinely new, but the path navigation portion could reuse jsonq to avoid duplication.

• locale/de/client.json has 66 additions / 63 deletions that appear to be unrelated reformatting — consider splitting that into a separate commit.

@JimKnoxx Hi there - there's a couple of things there to look at - the jsonq thing i'd deffo look at for code duplication. If you do need any additional json stuff that might also be reusable then adding it to that package might be a good move too - rather than keeping it with the oauth stuff?

@JimKnoxx I messed something up with the branch protection - so bear with me I'm sorting this now - you'll prb need to merge main back in when I've sorted it.

EDIT: I think all sorted now - some of the GH workflows weren't running, it was me trying to make things faster (and breaking things)

The role definition should now be per provider.

I also found some URLs that point nowhere - hope it's ok to fix it in this MR together with the improvements to the german translations.

@JimKnoxx I'm pretty happy with this I think - you happy with my last change?

Yes! Looks good to me and works, thanks for taking the time and refactor it again.

I noticed 2 more things I wanna fix, just FYI:

• The allowed_roles are also checked for fider administrator and collaborators. But they should be able to login regardless - to prevent lockouts.
• If a user has an active session and one would change the allowed roles to prevent login/usage from this user, the user would still be able to post/vote/comment.

@mattwoberts for the second problem:
The solution I would propose adds a new column that stores the roles a user had at the time of login. This column is updated on every new login. The stored roles are then checked against the allowed roles.

This means that if a role is removed in Fider, the change will be reflected immediately. However, if the user loses the role in the OAuth provider system during an active session, the change will not be detected immediately, and the user will still be able to use the system until they log in again.

Do you have an idea for this? Or would you say, this is good enough?

This means that if a role is removed in Fider, the change will be reflected immediately. However, if the user loses the role in the OAuth provider system during an active session, the change will not be detected immediately, and the user will still be able to use the system until they log in again.

Do you have an idea for this? Or would you say, this is good enough?

It feels like this is touching on some kind of security stamp type implementation. In .NET Core, there's a concept of a security stamp, which basically is a timestamp of the user's profile.If something changes, for example the user changes their password, then the security stamp is updated. At regular intervals, e.g., every 20 minutes. This is then checked and if it's changed, then the user is forced to log in again. So that might be something that would be worth doing. For example, if an admin changed their password on a different computer, it would then force a re-login on another computer that he might be logged in at.

However, it doesn't really solve this problem because we would need to know about the change to their roles from the oauth provider. So you would need some way for the OAuth provider to tell Fider that the roles have changed. You could however use a security stamp type solution with your suggested change of checking the roles from the OAuth provider on every new login. It's not going to be as instant but other alternatives quickly start to look quite messy.

@JimKnoxx getting my head around an april release, what's the status of this, is it something I should earmark for inclusion?

Thank you for your feedback to the security stamps.
I hope this is somewhat like you had it in mind?

@mattwoberts I think the server you ran tests against is offline?

@JimKnoxx Not looked much at the code, just testing it a bit, and if you change your own name, it will log you out on the browser / session you're on - the idea is that the security stamp forces you to log in on all OTHER sessions, not the one that did the action...

@mattwoberts I see, makes sense.

@mattwoberts Should work now as you intend

@JimKnoxx Ah, it's still not working for me - I think the mismatch is because you're working from the perspective of the oauth change, and I'm testing from the perspective of a "normal" login. So currently, if you login and change your username, you're logged out since your security stamp session is lost.

Thinking some more on this, I don't think a username / avatar change should really invalidate your security stamp. Typically on a "normal" app it would be role / password changes that would trip this. A name / avatar change shouldn't really affect the security stamp, so I'm going to remove this - let me know if you have an issue with that.

@mattwoberts Oh, actually I never tested changing profile settings, so yeah! Good catch. That definitely also should not end the session!

Ok I think this makes sense now...

@mattwoberts Noticed two more things:

• if setting the allowed roles first and then the roles path, the stamps would not be rerolled.
• if removing the allowed roles or the roles path and therefor deactivating the check, the users will not get logged out anymore (because all users are allowed, more a UX thing than a security thing)

@mattwoberts Checked your changes too and I think this is working good now. What do you think?

@JimKnoxx Ok I'll bring this in soon!